Two-step change for governable values #515
Conversation
The time delay ensures that users have more information regarding the state of the contract. This is to avoid situatoins where the state of a Deposit is unpredictable due to governance changes earlier within the same block.
| /// @notice Set the system signer fee divisor. | ||
| /// @dev This can be finalized by callingm `finalizeSignerFeeDivisorUpdate` |
| require( | ||
| block.timestamp.sub(signerFeeDivisorChangeInitiated) >= governanceTimeDelay, | ||
| "Timer not elapsed, or change not initiated" | ||
| ); |
There was a problem hiding this comment.
Could we extract this common part from finalize* functions to a modifier?
E.g.:
function finalizeSignerFeeDivisorUpdate()
external
onlyOwner
afterChangeDelay(signerFeeDivisorChangeInitiated)
{
signerFeeDivisor = newSignerFeeDivisor;
emit SignerFeeDivisorUpdated(newSignerFeeDivisor);
newSignerFeeDivisor = 0;
signerFeeDivisorChangeInitiated = 0;
}
modifier afterChangeDelay(uint256 _changeInitializedTimestamp) {
require(
block.timestamp.sub(_changeInitializedTimestamp) >=
governanceTimeDelay,
"Timer not elapsed, or change not initiated"
);
_;
}Of course needs replacing afterChangeDelay with some clever name.
| newSignerFeeDivisor = 0; | ||
| signerFeeDivisorChangeInitiated = 0; |
There was a problem hiding this comment.
What will happen if we call finalizeSignerFeeDivisorUpdate once again? Will signerFeeDivisor be set to 0?
Maybe we shouldn't do newSignerFeeDivisor = 0 here?
Or maybe we should check if signerFeeDivisorChangeInitiated > 0 at the beginning?
require(
_changeInitializedTimestamp > 0,
"Change not initiated"
);
require(
block.timestamp.sub(_changeInitializedTimestamp) >= governanceTimeDelay,
"Timer not elapsed"
);There was a problem hiding this comment.
Comment above refers also to other functions.
There was a problem hiding this comment.
yes, this we need
There was a problem hiding this comment.
@NicholasDotSol have you pushed any updates around that?
There was a problem hiding this comment.
yep, we have 2 separate requirements in onlyAfterDelay modifier now
| require(collateralizationThresholdsChangeInitiated > 0, "Update not initiated"); | ||
|
|
||
| uint256 elapsed = block.timestamp.sub(collateralizationThresholdsChangeInitiated); | ||
| return (elapsed >= governanceTimeDelay)? | ||
| 0: | ||
| governanceTimeDelay.sub(elapsed); |
There was a problem hiding this comment.
Can we extract common code to an internal function?
There was a problem hiding this comment.
sure, i'm ok with that because this isn't a user-facing function so the slightly extra cost doesn't matter much. also deployment should be cheaper
| describe("setSignerFeeDivisor", async () => { | ||
| it("sets the signer fee", async () => { | ||
| await tbtcSystem.setSignerFeeDivisor(new BN("201")) | ||
| describe("update Signer fee", async () => { |
There was a problem hiding this comment.
update Signer fee -> update signer fee?
Uninitiated change and unelapsed timer are separate test cases now.
Ensure the values are changes correctly
NicholasDotSol
force-pushed
the
two-step-gov
branch
from
f9b57f2
to
4eb270c
Compare
getRemainingUpdateTime functions share most of their functinality. Extract that functionality into an internal function.
NicholasDotSol
force-pushed
the
two-step-gov
branch
from
33673cf
to
d7adb82
Compare
Ensure the update overrides the old value and resets the timestamp. Time tests have a grac eperiod of 1 second to account for unexpected time incrememnts due due to computation time.
From 1 hour to 6
Adding the timestamp grace allows the test to be off by 1 due to execution time pushing the timestamp up by 1 unpredictably
…nto two-step-gov
closes: #493
Changing/upgrading things in the System contract without warning can lead to unpredictable function call behavior.
Make all upgrades require two steps with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period.